YAS-335 Improvements to Ethereum event endpoints #116

ben-chain · 2020-04-28T20:25:14Z

Description

Fixes YAS-335 improvements around event logging, particularly:

fixing and testing that multi-contract-event transaction logs are parsed correctly
fixing bug where contracts could emit events that looked like execution manager events and trick the handler into parsing false events on other contracts' behalf
fixing logIndex (not well tested but appears to work -- wasn't breaking anything anyway)

Questions

Caveats with the current setup:

At this time, we add the execution manager address to any filtering, which could end up consuming lots of resources unnecessarily.
At this time, we do not correctly parse events emitted with DELEGATECALL -- they will appear to be emitted by the callee's OVM address instead of the caller's OVM address.
Filtering by topic is not supported, though is likely high priority in terms of getting eth_getLogs up to par for Dev Ex. Appears not to be needed for the Graph, so creating a separate ticket for it.

Metadata

Fixes

Fixes YAS 335

Contributing Agreement

I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

K-Ho · 2020-04-29T20:32:10Z

packages/ovm/src/app/utils.ts

-        activeContract = executionManagerLog.values['_activeContract']
+    if (log.address.toUpperCase() === executionManagerAddress.toUpperCase()) {
+      const EMLogIndex = log.logIndex
+      if (EMLogIndex < prevEMLogIndex) {


should tihs be <=? E.g. if prevEMlogIndex is 0, and EMLogIndex is 0, it's still a new transaction, right?

nice catch!

K-Ho · 2020-04-29T20:33:03Z

packages/ovm/src/app/utils.ts

      }
    } else {
-      loggerLogs.push(`Non-EM log: ${JSON.stringify(log)}`)
-      ovmLogs.push({ ...log, address: activeContract })
+      const newIndex = log.logIndex - cumulativeTxEMLogIndices


👌👌👌👌👌

K-Ho · 2020-04-29T20:45:14Z

packages/rollup-full-node/src/app/web3-rpc-handler.ts

+    if (filter['topics']) {
+      if (filter['topics'].length > 1) {
+        // todo make this proper error
+        const msg = `The provided filter ${filter} has multiple levels of topic filter.  Multi-level topic filters are currently unsupported by the OVM.`


K-Ho · 2020-04-29T20:54:45Z

packages/rollup-full-node/test/app/web-rpc-handler.spec.ts

+          parsedLogs.length.should.eq(1)
+          parsedLogs[0].signature.should.eq(DUMMY_EVENT_NAME)
+        }
+        it('should return correct logs with #nofilter', async () => {


😂"#nofilter"

LMAO so glad you caught it

K-Ho · 2020-04-29T20:58:23Z

packages/rollup-full-node/test/contracts/transpiled/events/MasterEventEmitter.sol

+        sub = SubEventEmitter(_sub);
+    }
+    function callSubEmitter() public {
+        emit Taco(0x0000000000000000000000000000000000000000);


K-Ho · 2020-04-29T20:58:34Z

packages/rollup-full-node/test/contracts/transpiled/events/SubEventEmitter.sol

+    event Burger(address);
+
+    function doEmit() public {
+        emit Burger(0x4206900000000000000000000000000000000000);


K-Ho · 2020-04-29T21:00:10Z

packages/rollup-full-node/test/app/web-rpc-handler.spec.ts

+            Web3RpcMethods.getTransactionReceipt,
+            [tx.hash]
+          )
+          gotLogs.should.deep.equal(receipt.logs)
        })


Could you add one more test here that filters by topic "taco", and makes sure that only 1 log is returned, and it's the right one?

K-Ho

Looks great!!! just 1 more test suggestion, and that one ">=" note

willmeister · 2020-04-29T21:45:20Z

packages/ovm/src/app/abi.ts

+/* External Imports */
+import { utils, ContractFactory } from 'ethers'
+
+/* Internal Imports */
+import * as ExecutionManager from '../../build/contracts/ExecutionManager.json'
+
+const EMContract = new ContractFactory(
+  ExecutionManager.abi,
+  ExecutionManager.bytecode
+)
+const EMEvents = EMContract.interface.events
+const topics = []
+for (const eventKey of Object.keys(EMEvents)) {
+  topics.push(EMEvents[eventKey].topic)
+}
+
+export const ALL_EXECUTION_MANAGER_EVENT_TOPICS = topics


could this go in utils.ts? It has and exports abi stuff. If nothing else, maybe reference it instead of importing the contract?

Happy to move to utils, but what do you mean by "reference instead of import"?

Just to export from here and reference. Not a big priority though.

willmeister · 2020-04-29T21:50:05Z

packages/ovm/src/app/utils.ts

+      const executionManagerLog = executionManagerInterface.parseLog(log)
+      if (!executionManagerLog) {
+        loggerLogs.push(
+          `execution manager log ${log} was unrecognized by the interface parser--Definitely not an activeContract event, ignoring...`


I think ${log} will just print [object Object], but JSON.stringify(...) will result in circular error. Is there another property that would be useful to log here? log.name?

willmeister · 2020-04-29T21:52:19Z

packages/rollup-full-node/src/app/web3-rpc-handler.ts

+    if (filter['topics']) {
+      if (filter['topics'].length > 1) {
+        // todo make this proper error
+        const msg = `The provided filter ${filter} has multiple levels of topic filter.  Multi-level topic filters are currently unsupported by the OVM.`


Should you log ${JSON.stringify(filter)}? It's probably just going to print [object Object] otherwise.

b5b564702 popm/wasm: add wasm-opt target to optimise wasm binary (ethereum-optimism#146) 27a5081e3 tbc: allow seeds to be overwritten (ethereum-optimism#158) 18afbc403 Fix missing panic (ethereum-optimism#156) 05cee0afb tbc: remove height as the terminal condition for indexers (ethereum-optimism#152) 4402060d6 Use hemilabs/websocket fork of nhooyr.io/websocket (ethereum-optimism#153) 3df5001c4 Add initial CODEOWNERS file (ethereum-optimism#149) a4685a57f popm/wasm: improve and tidy up Go code (ethereum-optimism#144) 41a0009c1 Add forking detection to TBC (ethereum-optimism#101) bbeed8bac ignore ulimits in tbc when on localnet (ethereum-optimism#142) 4e1914cc6 Stopgap to re-fetch blocks with no children (ethereum-optimism#131) 12eefff06 e2e: fix issues detected by staticcheck (ethereum-optimism#134) 80153372f Add more documentation for TBC and related RPC protocol (ethereum-optimism#86) 723b63704 bfg: fix issues reported by staticcheck (ethereum-optimism#132) 509fb1be6 electrumx: fix unhandled error in NewJSONRPCRequest (ethereum-optimism#133) b19af1e1f hemictl: use sort.Strings(...) instead of sort.Sort(sort.StringSlice(...)) (S1032) (ethereum-optimism#123) cf7c570f9 popmd: remove unused 'handle' func and return err in connectBFG (ethereum-optimism#124) fa2a4c4de ci: fix bug that makes version type always 'unstable' (ethereum-optimism#125) ddc39655a ci: use org DOCKERHUB_TOKEN secret (ethereum-optimism#128) 6457d16f5 bfgd: drop btc_blocks_can refresh triggers for l2_keystones/pop_basis (ethereum-optimism#120) 2b3c096bd popm: simplify receivedKeystones variable in tests (ethereum-optimism#116) 6140bbf24 tbc: move RPC tests to a separate test file (ethereum-optimism#114) c76571ff0 Add configuration option for static fees to web PoP miner (ethereum-optimism#119) 233817e65 popm: use simple conversion instead of unnecessary fmt.Sprintf (S1025) (ethereum-optimism#115) 2879f5fa7 e2e: improve name for variable with type `time.Duration` (ST1011) (ethereum-optimism#117) 4f31965e6 database,e2e: remove use of deprecated io/ioutil (SA1019) (ethereum-optimism#118) e9e090696 tbc: do not recreate outpoint for ScriptHashByOutpoint. (ethereum-optimism#109) 52eefb136 ignore l2 keystones notifications in tests (ethereum-optimism#112) eb607994c Sync Docker image environment variables with daemon configs (ethereum-optimism#111) 57281bc5d added a way to monitor and sanity-test localnet (ethereum-optimism#106) ea1a1c94c bfgd: fix loops unconditionally exited after one interation (SA4004) (ethereum-optimism#108) 29f116fb4 Add pprof http server to daemons (ethereum-optimism#105) 18a315d7e Added README for generating forks in BTC regtest for TBC fork resolution testing (ethereum-optimism#100) 55b8f52cb more robust nextPort (ethereum-optimism#103) 48f8b293f tbcapi: use reverse byte order for hashes in serialised, tidy up (ethereum-optimism#104) b7e9f5ecb restart initialblocks on-failure (ethereum-optimism#102) f9d52d423 Update required Go version to v1.22.2 (ethereum-optimism#96) f6808aa5b Fix op-proposer op-node dependency condition (fixes ethereum-optimism#98) (ethereum-optimism#99) a882529e8 Kill all pending block downloads if a peer fails (ethereum-optimism#95) eb66345e1 e2e: tidy up docker-compose file (ethereum-optimism#81) 34766f725 Track pending blocks with ttl package (ethereum-optimism#90) 6ed3eb88b Added configurable fee-per-vB to PoP Miner (ethereum-optimism#91) 509e31fbc Replace os.Kill with syscall.SIGTERM in signal.Notify calls (ethereum-optimism#87) git-subtree-dir: heminetwork git-subtree-split: b5b564702e8d3bedcdf0e0a52c22e383d7fd4dbe

…sschain-events fix: merge develop to crosschain events

ben-chain added 2 commits April 28, 2020 16:17

everything working

571a68a

linting

cce8691

ben-chain changed the title ~~YAS-335 Logging improvements~~ YAS-335 Improvements to Ethereum event endpoints Apr 28, 2020

ben-chain added 6 commits April 28, 2020 16:44

linting

9d3fd63

bad multi level filter

7c1b3cc

handle unsupported filter handler

021917a

merge updated error code

ba3f5de

linting

ebd81f3

merge master

ec8c861

K-Ho reviewed Apr 29, 2020

View reviewed changes

ben-chain added 4 commits April 29, 2020 16:55

fix merge accidents

36ee32e

linting

b4bddc1

linting

ee41892

fix log indexing bug

96cb62a

K-Ho reviewed Apr 29, 2020

View reviewed changes

K-Ho approved these changes Apr 29, 2020

View reviewed changes

ben-chain added 3 commits April 29, 2020 17:11

add filtering multi-event test

051bbfd

linting

7baf8d2

linting

212f700

willmeister reviewed Apr 29, 2020

View reviewed changes

ben-chain added 3 commits April 29, 2020 18:11

use already existing EM interface

9b8180c

linting

04fa273

linting

6c4914d

ben-chain merged commit 6a8a7a8 into master Apr 29, 2020

gakonst deleted the bug/eth_getLogs/incorrect-address branch March 18, 2021 15:01

bap2pecs pushed a commit to babylonlabs-io/optimism that referenced this pull request Jul 31, 2024

feat: Add remote eots manager (ethereum-optimism#116)

6858a1b

maurelian pushed a commit that referenced this pull request Oct 23, 2024

Merge pull request #116 from defi-wonderland/fix/merge-develop-to-cro…

3fd3d7b

…sschain-events fix: merge develop to crosschain events

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

YAS-335 Improvements to Ethereum event endpoints #116

YAS-335 Improvements to Ethereum event endpoints #116

ben-chain commented Apr 28, 2020 •

edited

Loading

K-Ho Apr 29, 2020

ben-chain Apr 29, 2020

K-Ho Apr 29, 2020

K-Ho Apr 29, 2020

K-Ho Apr 29, 2020

ben-chain Apr 29, 2020

K-Ho Apr 29, 2020

K-Ho Apr 29, 2020

K-Ho Apr 29, 2020

K-Ho left a comment •

edited

Loading

willmeister Apr 29, 2020

ben-chain Apr 29, 2020

willmeister Apr 29, 2020

willmeister Apr 29, 2020

willmeister Apr 29, 2020

YAS-335 Improvements to Ethereum event endpoints #116

YAS-335 Improvements to Ethereum event endpoints #116

Conversation

ben-chain commented Apr 28, 2020 • edited Loading

Description

Questions

Metadata

Fixes

Contributing Agreement

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K-Ho left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-chain commented Apr 28, 2020 •

edited

Loading

K-Ho left a comment •

edited

Loading